-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Rename database from 'couchbasedb' to 'couchbase' in documentation and db_engine_specs #29911
Conversation
…nd db_engine_specs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29911 +/- ##
===========================================
+ Coverage 60.48% 83.70% +23.21%
===========================================
Files 1931 527 -1404
Lines 76236 38055 -38181
Branches 8568 0 -8568
===========================================
- Hits 46114 31853 -14261
+ Misses 28017 6202 -21815
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some issues reported by CI. In addition, the breaking change is problematic, and would need to at least be addressed in UPDATING.md
and/or by adding an alias to the old name.
@@ -102,7 +102,7 @@ | |||
"clickhouse": Dialects.CLICKHOUSE, | |||
"clickhousedb": Dialects.CLICKHOUSE, | |||
"cockroachdb": Dialects.POSTGRES, | |||
"couchbasedb": Dialects.MYSQL, | |||
"couchbase": Dialects.MYSQL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would entail a breaking change, and would break every Couchbase connection created by existing users. At minimum we would need to create an alias for the old dialect name to ensure old connections still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have corrected this in new patch. I am adding alias to old name also. I am seeing all checks passed too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am adding alias to old name also.
I don't see the alias here - can you check how the alias is implemented in the Postgres spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake I thought alias means updating sql_parse , I saw oceanbase and postgres db_engine_specs and similar to that I have added engine_aliases = {"couchbase", "couchbasedb"}
to db_engine_spec/couchbase.py .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second pass comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit, after that this LGTM 👍
engine = "couchbasedb" | ||
class CouchbaseEngineSpec(BasicParametersMixin, BaseEngineSpec): | ||
engine = "couchbase" | ||
engine_aliases = {"couchbase", "couchbasedb"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You donät need the main name in the alias set, this should be enough:
engine_aliases = {"couchbase", "couchbasedb"} | |
engine_aliases = {"couchbasedb"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 failed checks, but they were not failing earlier, are they because of my patch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failed MySQL test is flaky - we can restart the test. The docs one seems unrelated - It seems resources/INTHEWILD.md
has a link to /www.cnovit.com
that isn't working. I think we may need to remove the link if the homepage isn't up anymore. Do you want to do it in this PR? I can also open a PR to fix it if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix in this PR. I will upload a new patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@michael-s-molina this will not be a breaking change as per the last changes. So I think it's safe to remove that label. |
@@ -80,7 +80,6 @@ Join our growing community! | |||
- [Caizin](https://caizin.com/) [@tejaskatariya] | |||
- [Careem](https://www.careem.com/) [@SamraHanifCareem] | |||
- [Cloudsmith](https://cloudsmith.io) [@alancarson] | |||
- [CnOvit](http://www.cnovit.com/) [@xieshaohu] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this removal intentional? Maybe you meant to add Couchbase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link is not working. So tests were failing. @villebro is discussing about it in previous conversation. So I removed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayush-couchbase it seems the link was failing due to being http
and not https
- could you update that? The https
one works for me at least. After that I think we're good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure , Updated in new patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the link checker does not block merging... it just gives us little to-do items as maintainers. I'll open a PR today to sweep up whatever's left at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for all the patience and iterations!
FYI @sadpandajoe the author would like to get this into the next release opportunity. I tagged it as 4.1 in case there's an RC4, but let me know if there's anything else I should do to get it into 4.1.1 if RC3 passes the vote. |
SUMMARY
Changing Name from couchbasedb to couchbase. Makes it more clear and logical about underlying database.
No major change done.
TESTING INSTRUCTIONS
mannual testing
ADDITIONAL INFORMATION